Add first class component cache#2126
Add first class component cache#2126reeganviljoen wants to merge 110 commits intoViewComponent:mainfrom
Conversation
7544079 to
7533a9c
Compare
|
@joelhawksley @Spone @camertron I would appreciate any thoughts on this. |
There was a problem hiding this comment.
@reeganviljoen THANK YOU for taking a crack at building something functional to address the long-overdue need for caching in ViewComponent.
To be honest with you, I have not used caching in Rails nearly enough to feel like I can properly assess this PR in terms of how it would integrate with the existing mental model of caching in Rails. I'm happy to learn that context, but I won't have the space to do so for at least a couple of weeks.
@Spone @boardfish have y'all done much with Rails' caching? @reeganviljoen if you know of anyone else in the community who would be a good reviewer here, I'd be happy to have them.
@joelhawksley I really aprreciate the honesty, I will see if I can find anyone with cache experience to take a crack at reviewing this ❤️ |
|
@joelhawksley Afraid it's not something I've really interacted with directly until now, sorry! I'd be coming from a similar place. Great to see @reeganviljoen working at it, though ✨ |
JWShuff
left a comment
There was a problem hiding this comment.
Reegan, really excited to see this. I'm going to take a shot at adapting our gem spec suite to your fork and open a PR at your fork with those specs by way of giving us a place to start that gives us the most cache functionality we can get.
lib/view_component/base.rb
Outdated
| include ViewComponent::Slotable | ||
| include ViewComponent::Translatable | ||
| include ViewComponent::WithContentHelper | ||
| include ViewComponent::Cacheable |
There was a problem hiding this comment.
Suggestion, nitpick and nonblocking: Can we slide this in alphabetically above? I'd think it'd be ~ line 32
| __vc_cache_dependencies.push(*args) | ||
| end | ||
|
|
||
| def inherited(child) |
There was a problem hiding this comment.
We took this for a spin in a few cases of our cached partials and view components that we are familiar with and largely it works for the base cases. This is working to correctly bust a cached VC when it changes or things in the render path 'above' (partials or VCs) it change.
What we aren't seeing is when a VC renders another VC or partial as a child that downstream changes of the cached VC pick up changes and handle it. I'm going to fork your branch and offer some test cases based on an approach we use in the vc fragment caching gem (what we're currently solving this with)
Effectively we want some way for a child partial or VC change (in either the .rb or template) to bust the cached parent.
There was a problem hiding this comment.
also big thanks for testing it, I can look at adding the touch true thing later this week
There was a problem hiding this comment.
@reeganviljoen I'd expect us to test this with changes to child partials.
JWShuff
left a comment
There was a problem hiding this comment.
Added a hypothetical approach to an integration test for the behavior.
There was a problem hiding this comment.
@reeganviljoen I am not a git-wizard, much to my eternal shame, but I set up a fork and adjusted the spec approach here to use the integration examples/controllers to assert the behavior. Feel free to take, leave, or otherwise.
There's a wider challenge around partial/template digesting, and on a quiet day I'll port our spec suite over to the VC style and get it implemented so we have all the permutations we know of that need to cache appropriately.
|
@JWShuff I investigated a few of your suggestions, added a few, thanks, and others I am struggling with |
|
@joelhawksley with a live use case tested how do you feel about this now |
cb357ca to
06bc05a
Compare
…or potential adoption
…nstead of custom implementation.
Co-authored-by: Joel Hawksley <joelhawksley@github.com>
|
@joelhawksley I also added a benchmark for use to have an idea of the gain/overheads of this |
|
After a few refactors to improve the benchmark numbers it now looks a lot better |
|
and finnaly I realised the reason the cache benchmarks were worse is because in casses where the coomponent is not expensive to render cache will most likely be too expensive therefore I added more complexitiy to the benchmark components and these are the results |
|
@joelhawksley im not sure if thd primer failure is related but everything I feel is done, I would appreciate a review when ever you can |
closes #234
What are you trying to accomplish?
Add experimental fragment caching for ViewComponent that is true to the ViewComponent ethos, namely:
What approach did you choose and why?
I chose to add an opt-in caching API:
ViewComponent::Cacheable+ acache_onmacro that declares which instance methods contribute to the cache keyActiveSupport::Cache.expand_cache_key) + a component digestrenderdependenciesERB::Compiler(no Temple)Anything you want to highlight for special attention from reviewers?
require "view_component/fragment_caching"or per-componentinclude ViewComponent::Cacheable)cache_onsupports private methods and uses Rails cache key expansion for dependency valuesRAILS_CACHE_ID/RAILS_APP_VERSIONon deploy)